Add ExtensionFilter API to extensions#3650
Conversation
istio-testing
added
the
do-not-merge/work-in-progress
|
😊 Welcome @liamawhite! This is either your first contribution to the Istio api repo, or it's been You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines Thanks for contributing! Courtesy of your friendly welcome wagon. |
istio-testing
added
the
size/XL
- Move top-level TrafficSelector to extension_filter.proto - Keep WasmPlugin.TrafficSelector nested for backward compat - Remove targetRef from ExtensionFilter - Add release notes
istio-policy-bot
added
the
lifecycle/stale
istio-policy-bot
removed
the
lifecycle/stale
liamawhite
changed the title
liamawhite
removed
the
do-not-merge/work-in-progress
| // +cue-gen:ExtensionFilter:storageVersion | ||
| // +cue-gen:ExtensionFilter:annotations:helm.sh/resource-policy=keep | ||
| // +cue-gen:ExtensionFilter:labels:app=istio-pilot,chart=istio,heritage=Tiller,release=istio | ||
| // +cue-gen:ExtensionFilter:subresource:status |
There was a problem hiding this comment.
Are we really going to write status to this? Or is the idea to just have it in case?
There was a problem hiding this comment.
The existing WASMPlugin API has validation written to status for things like missing URL, similar logic here. No conditions though by the looks of it.
| repeated TrafficSelector match = 6; | ||
|
|
||
| // WebAssembly filter configuration. Mutually exclusive with `lua`. | ||
| WasmConfig wasm = 10; |
There was a problem hiding this comment.
Can we not use a oneof here? It would also be good to have some documentation somewhere (not necessarily here) saying that admins who want to disallow certain types can deploy a VAP (and then provide a sample VAP)
There was a problem hiding this comment.
I see both oneof and CEL verification across the api and its not clear to me why to use one vs the other so I went with what the WASMPlugin API already did. Maybe CEL validation is used when there are back compat problems (if fields are added later)?
I tried with oneof and it seemed to generate everything fine so I guess that's preferable?
There was a problem hiding this comment.
@howardjohn can correct me if I'm wrong but I think oneoef is preferable for new APIs
There was a problem hiding this comment.
I think its probably good to use a oneof but it doesn't practically matter since its not user facing
| repeated TrafficSelector match = 6; | ||
|
|
||
| // WebAssembly filter configuration. Mutually exclusive with `lua`. | ||
| WasmConfig wasm = 10; |
There was a problem hiding this comment.
Why the skip in field numbers?
There was a problem hiding this comment.
I've seen it in other places in the API where some numbers are skipped to leave place for additional fields later. I think the argument is that its easy to keep track of what numbers are used if the fields are always increasing?
I can change it though if you want?
There was a problem hiding this comment.
Yeah let's keep it sequential; I think the reason for other apis skipping is past changes
| // that defines the appropriate callback functions (`envoy_on_request` and/or | ||
| // `envoy_on_response`). | ||
| // | ||
| // The maximum size is 64KB. For larger or more complex filters, use a |
There was a problem hiding this comment.
Is this enforced by the proxy or the control plane? Any way k8s could validate for us?
There was a problem hiding this comment.
The 64kb limit is somewhat arbitrary but overall size does have an impact on LDS response size and how much memory each proxy consumes (AFAICT envoy loads the script into memory once per worker thread). The only actual limit I can think of is the limit imposed by etcd, which I believe is 2mb?
What would you recommend we do here?
There was a problem hiding this comment.
Big +1 on starting with the limit. Let's just make sure we enforce it somewhere
| // metadata, and other request/response attributes. See the Envoy Lua filter | ||
| // documentation for the complete API: | ||
| // https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/lua_filter | ||
| message LuaConfig { |
There was a problem hiding this comment.
Did we decide if/how this CRD would/would not interact with per-route overrides?
There was a problem hiding this comment.
Also, do we support loading lua from a file with EnvoyFilter? Should we support it here?
…mment - Renumber ExtensionFilter fields to be contiguous (targetRefs=2, phase=3, priority=4, match=5) - Wrap wasm/lua in oneof filter_config for type-safe mutual exclusion - Clarify priority tiebreaker ordering (creationTimestamp, then name/namespace) - Regenerate pb.go, pb.html, and CRD YAML
5 participants
Please provide a description of this PR:
Adds native support for Lua filters as described in the following design doc.
Related implementation PR is here.